feat: add Bun TypeScript SDK as a workspace package - #18
Conversation
`ignite serve` gave `--host` an auto-derived `-h`, which collides with
clap's generated `--help`. Under debug assertions that panics before a
single argument is parsed, so `cargo run -- serve` never started; release
builds silently rebound `-h` to `--host` instead. Move host to `-H`, which
is what the man page already documented, and add a regression test that
runs clap's `debug_assert()` over the whole command tree.
Default the daemon to port 9847 in both the CLI and the standalone HTTP
binary. 3000 is heavily contested on a developer machine, and the port is
now shared with the TypeScript SDK's default so neither side needs
configuring.
`execute_service` is synchronous and runs for the entire lifetime of the
microVM, but was called directly from an async handler. That parked a
Tokio worker thread for the whole execution, so enough concurrent calls
starved the runtime and stalled every other route including /health. Move
it onto `spawn_blocking` and surface `JoinError` rather than reporting a
success with no metrics.
`list_services` swallowed `read_dir` errors and returned `{"services":[]}`
with HTTP 200, making a misconfigured `--services` path indistinguishable
from an empty directory. Report the failure with the path and cause, and
sort results so the endpoint is deterministic.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds @ignite/sdk under sdk/ts, wired up as a Bun workspace at the repo
root so `bun install` produces a committed lockfile and dependencies
resolve inside the repository.
Type resolution is pinned via typeRoots. Without it tsc walks past the
repository root and can satisfy @types/bun from a global node_modules,
which makes `typecheck` pass locally and fail in CI. With node_modules
removed the typecheck now fails as it should.
The package emits real build output: `exports`/`types`/`files` plus a
tsconfig.build.json emitting dist/ with declarations and sourcemaps.
Relative imports carry .js extensions so the emitted ESM resolves under
plain Node, not just Bun.
The client covers what the daemon actually does:
- Timeouts and AbortSignal on every method. executeService boots a
microVM so it gets a 5 minute budget against 30s for metadata calls,
both overridable, 0 to disable. A timeout raises IgniteTimeoutError
distinct from a caller abort, because a client-side timeout does not
mean the guest stopped running.
- Both daemon error shapes: the ExecuteResponse body and the bare
{"error"} used for 401 and 429, which carries no success field.
- Service names validated locally against the same rules as
validate_service_name, rejecting before spending a round trip.
- input typed as JsonValue. It was Record<string, unknown> | unknown,
a union that collapses to unknown and checked nothing.
Tests cover auth and rate-limit shapes, non-JSON error bodies,
success:false on HTTP 200, timeouts, aborts, and header handling. An
opt-in suite gated on IGNITE_TEST_BASE_URL runs against a live daemon,
since the unit tests assert against hand-written JSON and only that
suite proves the types match what the daemon emits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI and HTTP server now default to port 9847 and improve service listing and execution handling. A new TypeScript SDK provides typed client methods, errors, timeouts, cancellation, examples, tests, and package documentation. ChangesIgnite HTTP API and TypeScript SDK
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Example
participant IgniteClient
participant IgniteHTTP
participant IgniteService
Example->>IgniteClient: Check health and list services
IgniteClient->>IgniteHTTP: Send JSON request
IgniteHTTP-->>IgniteClient: Return API response
Example->>IgniteClient: Execute selected service
IgniteClient->>IgniteHTTP: Send execution request
IgniteHTTP->>IgniteService: Run validated service
IgniteService-->>IgniteHTTP: Return execution report
IgniteHTTP-->>IgniteClient: Return result or error
IgniteClient-->>Example: Report metrics, output, or typed failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
package.json (1)
9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise
engines.bunto the version that supportsbun run --filter.The workspace scripts use
bun run --filter '*' ..., and Bun added--filterin v1.1.4. Keeppackage.jsonfrom accepting Bun 1.0.x unless the SDK scripts are changed to use only flags available in that floor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 9 - 16, Update the package.json engines.bun constraint to require Bun 1.1.4 or newer, matching the --filter usage in the build, test, and typecheck scripts; do not alter the scripts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ignite-http/src/server.rs`:
- Around line 170-182: Update the directory iteration in the service-listing
handler to process each ReadDir result explicitly instead of using
entries.flatten(). On any iteration io::Error, return the same HTTP 500 response
path used for the initial read_dir failure; otherwise preserve the existing
directory filtering and deterministic services.sort() behavior.
- Around line 453-470: Update the test
list_services_reports_an_unreadable_directory_instead_of_an_empty_list to create
a tempfile::tempdir() and derive a guaranteed-missing child path from it,
replacing the hard-coded absolute PathBuf while preserving the existing
assertions.
- Around line 259-286: Extend ServerState with a shared semaphore sized to the
allowed concurrent service executions, and initialize it wherever the state is
constructed. In execute_service_handler, call try_acquire_owned() before
spawn_blocking; return the established 429 or 503 response when no permit is
available. Move the owned permit into the blocking closure so it remains held
through execute_service completion, including error and panic paths.
In `@sdk/ts/src/client.ts`:
- Line 232: Update executeService in sdk/ts/src/client.ts:232-232 to resolve
timeouts using request options first, then the explicitly provided constructor
timeout (preserving timeoutMs: 0), and finally the default; ensure
explicitTimeoutMs records whether the constructor option was supplied and add a
unit test asserting IgniteTimeoutError.timeoutMs. Update the timeoutMs
documentation in sdk/ts/src/types.ts:101-107 to describe the actual precedence,
and revise sdk/ts/README.md:95-105 to state that execution budgets are
configurable per request.
- Around line 167-177: Update the health() method to validate that body is a
valid HealthResponse-shaped object before casting or returning it, matching the
parsed-shape guards used by listServices and executeService. Treat undefined,
null, and non-object bodies as an error and preserve the existing IgniteApiError
response details.
---
Nitpick comments:
In `@package.json`:
- Around line 9-16: Update the package.json engines.bun constraint to require
Bun 1.1.4 or newer, matching the --filter usage in the build, test, and
typecheck scripts; do not alter the scripts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8a2603e-383a-4a56-8fcd-74a4a5d69197
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.mddocs/api.mddocs/getting-started.mddocs/man/ignite.1docs/walkthrough.mdignite-cli/src/main.rsignite-http/Cargo.tomlignite-http/src/main.rsignite-http/src/server.rspackage.jsonsdk/ts/README.mdsdk/ts/example.tssdk/ts/package.jsonsdk/ts/src/client.tssdk/ts/src/errors.tssdk/ts/src/index.tssdk/ts/src/types.tssdk/ts/test/client.test.tssdk/ts/test/integration.test.tssdk/ts/tsconfig.build.jsonsdk/ts/tsconfig.json
| let mut services = Vec::new(); | ||
| if let Ok(entries) = fs::read_dir(&state.services_path) { | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if let Some(name) = path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .filter(|_| path.is_dir()) | ||
| { | ||
| services.push(name.to_string()); | ||
| } | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if let Some(name) = path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .filter(|_| path.is_dir()) | ||
| { | ||
| services.push(name.to_string()); | ||
| } | ||
| } | ||
| Json(serde_json::json!({ "services": services })) | ||
| // Directory order is filesystem-dependent; sort so the API is deterministic. | ||
| services.sort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return an error for directory iteration failures.
Line 171 discards ReadDir errors with flatten(). If iteration fails after the directory opens, this endpoint returns a partial service list with HTTP 200. Handle each Result<DirEntry, io::Error> and return the same HTTP 500 response used for the initial read_dir failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 170 - 182, Update the directory
iteration in the service-listing handler to process each ReadDir result
explicitly instead of using entries.flatten(). On any iteration io::Error,
return the same HTTP 500 response path used for the initial read_dir failure;
otherwise preserve the existing directory filtering and deterministic
services.sort() behavior.
| // `execute_service` is synchronous and runs for the entire lifetime of the | ||
| // microVM. Calling it directly from an async handler parks a Tokio worker | ||
| // thread for that whole duration, so enough concurrent executions starve | ||
| // the runtime and stall every other route, including `/health`. | ||
| let result = | ||
| tokio::task::spawn_blocking(move || execute_service(&service_dir, options, None, None)) | ||
| .await; | ||
|
|
||
| let result = match result { | ||
| Ok(result) => result, | ||
| Err(join_err) => { | ||
| // The blocking task panicked or was cancelled. Surface it rather | ||
| // than reporting success with no metrics. | ||
| tracing::error!(error = %join_err, "service execution task failed to complete"); | ||
| return ( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| Json(ExecuteResponse { | ||
| success: false, | ||
| service_name, | ||
| metrics: None, | ||
| preflight: None, | ||
| error: Some(format!("Execution task failed to complete: {join_err}")), | ||
| }), | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| match result { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the declared Tokio version and all execution-admission controls.
rg -n -C 4 \
-g 'Cargo.toml' -g 'Cargo.lock' -g '*.rs' \
'tokio|spawn_blocking|Semaphore|execute_service_handler|struct ServerState' .Repository: dev-dami/ignite
Length of output: 12060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo workspace Tokio dependency =="
sed -n '1,30p' Cargo.toml
echo
echo "== ServerState definition and execute handler outline =="
sed -n '1,45p' ignite-http/src/server.rs
echo
sed -n '200,295p' ignite-http/src/server.rs
echo
echo "== Existing Semaphore/Concurrency controls =="
rg -n '\bSemaphore\b|semaphore|try_acquire|acquire_owned|tokio::sync' ignite-http/src/server.rs ignition-http ignite-shared ignite-core ignite-cli || true
echo
echo "== Runtime/blocking-task settings around Tokio main and serve =="
sed -n '1,90p' ignite-http/src/main.rs
echo
echo "== Static probe: count spawn_blocking under execute_service_handler and ServerState fields =="
python3 - <<'PY'
from pathlib import Path
p=Path('ignite-http/src/server.rs')
text=p.read_text()
start=text.find(' async fn execute_service_handler')
end=text.find('\n pub fn create_router', start)
body=text[start:end] if start!=-1 and end!=-1 else ''
print("spawn_blocking_count=", body.count('spawn_blocking'))
print("ServerState fields in file:")
for i,line in enumerate(Path('ignite-http/src/server.rs').read_text().splitlines(),1):
if 'pub struct ServerState' in line or 'Semaphore' in line or 'RateLimiter' in line:
print(f"{i}: {line}")
PYRepository: dev-dami/ignite
Length of output: 8973
Bound concurrent service executions before spawn_blocking.
ServerState only keeps the request rate limiter and has no execution semaphore. execute_service_handler queues a blocking microVM task for every accepted request, with no Tokio maximum-blocking limit configured. Add a shared bounded execution semaphore, use try_acquire_owned() before scheduling work, return 429 or 503 when capacity is full, and keep the permit until execute_service completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 259 - 286, Extend ServerState with a
shared semaphore sized to the allowed concurrent service executions, and
initialize it wherever the state is constructed. In execute_service_handler,
call try_acquire_owned() before spawn_blocking; return the established 429 or
503 response when no permit is available. Move the owned permit into the
blocking closure so it remains held through execute_service completion,
including error and panic paths.
| #[tokio::test] | ||
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | ||
| // Previously this swallowed the error and returned `{"services": []}` | ||
| // with HTTP 200, making a misconfigured path look like an empty one. | ||
| let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist"); | ||
|
|
||
| let (status, body) = get(test_state(missing), "/services").await; | ||
|
|
||
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | ||
| assert!( | ||
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | ||
| "expected a read failure message, got {body}" | ||
| ); | ||
| assert!( | ||
| body.get("services").is_none(), | ||
| "a failed listing must not report a services array" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a temporary path for the missing-directory test.
Line 457 embeds an absolute host path. The path can exist on a test host and makes the test depend on host filesystem layout. Create a missing child under tempfile::tempdir() instead.
As per coding guidelines: “Never introduce secrets, tokens, or host-specific paths into committed Rust code.”
Proposed fix
- let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist");
+ let dir = tempfile::tempdir().unwrap();
+ let missing = dir.path().join("missing");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tokio::test] | |
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | |
| // Previously this swallowed the error and returned `{"services": []}` | |
| // with HTTP 200, making a misconfigured path look like an empty one. | |
| let missing = PathBuf::from("/nonexistent/ignite-services-should-not-exist"); | |
| let (status, body) = get(test_state(missing), "/services").await; | |
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | |
| assert!( | |
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | |
| "expected a read failure message, got {body}" | |
| ); | |
| assert!( | |
| body.get("services").is_none(), | |
| "a failed listing must not report a services array" | |
| ); | |
| } | |
| #[tokio::test] | |
| async fn list_services_reports_an_unreadable_directory_instead_of_an_empty_list() { | |
| // Previously this swallowed the error and returned `{"services": []}` | |
| // with HTTP 200, making a misconfigured path look like an empty one. | |
| let dir = tempfile::tempdir().unwrap(); | |
| let missing = dir.path().join("missing"); | |
| let (status, body) = get(test_state(missing), "/services").await; | |
| assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR); | |
| assert!( | |
| body["error"].as_str().unwrap_or("").contains("Cannot read"), | |
| "expected a read failure message, got {body}" | |
| ); | |
| assert!( | |
| body.get("services").is_none(), | |
| "a failed listing must not report a services array" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ignite-http/src/server.rs` around lines 453 - 470, Update the test
list_services_reports_an_unreadable_directory_instead_of_an_empty_list to create
a tempfile::tempdir() and derive a guaranteed-missing child path from it,
replacing the hard-coded absolute PathBuf while preserving the existing
assertions.
Source: Coding guidelines
| const body = await IgniteClient.readBody(response); | ||
| if (!response.ok) { | ||
| throw new IgniteApiError( | ||
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | ||
| response.status, | ||
| undefined, | ||
| body, | ||
| ); | ||
| } | ||
| return body as HealthResponse; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the health body before the cast.
readBody returns undefined for an empty body. If the daemon answers 200 with an empty or non-object body, health() resolves to undefined while the declared return type is HealthResponse. The caller then throws a TypeError on health.status, as in sdk/ts/example.ts line 20.
listServices and executeService already guard the parsed shape. Apply the same guard here.
🛡️ Proposed fix
const body = await IgniteClient.readBody(response);
if (!response.ok) {
throw new IgniteApiError(
IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`),
response.status,
undefined,
body,
);
}
+ if (!body || typeof body !== 'object') {
+ throw new IgniteApiError(
+ 'Invalid JSON response from server during health check',
+ response.status,
+ undefined,
+ body,
+ );
+ }
return body as HealthResponse;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const body = await IgniteClient.readBody(response); | |
| if (!response.ok) { | |
| throw new IgniteApiError( | |
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| return body as HealthResponse; | |
| } | |
| const body = await IgniteClient.readBody(response); | |
| if (!response.ok) { | |
| throw new IgniteApiError( | |
| IgniteClient.errorMessage(body, `Health check failed with status ${response.status}`), | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| if (!body || typeof body !== 'object') { | |
| throw new IgniteApiError( | |
| 'Invalid JSON response from server during health check', | |
| response.status, | |
| undefined, | |
| body, | |
| ); | |
| } | |
| return body as HealthResponse; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/ts/src/client.ts` around lines 167 - 177, Update the health() method to
validate that body is a valid HealthResponse-shaped object before casting or
returning it, matching the parsed-shape guards used by listServices and
executeService. Treat undefined, null, and non-object bodies as an error and
preserve the existing IgniteApiError response details.
| audit: options.audit ?? false, | ||
| }), | ||
| }, | ||
| options.timeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constructor timeoutMs never applies to executeService. The root cause is at sdk/ts/src/client.ts line 232: executeService resolves its budget from options.timeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS and skips this.timeoutMs. A caller who sets new IgniteClient({ timeoutMs: 0 }) still gets a 5-minute execution timeout, and a caller who sets a shorter client budget still waits 5 minutes. The documentation in two other files states the opposite.
sdk/ts/src/client.ts#L232-L232: use the constructor value when it is set, for exampleoptions.timeoutMs ?? this.explicitTimeoutMs ?? DEFAULT_EXECUTE_TIMEOUT_MS, whereexplicitTimeoutMsrecords whether the caller passedtimeoutMs. Add a unit test that sets a client-leveltimeoutMsand asserts the reportedIgniteTimeoutError.timeoutMsforexecuteService.sdk/ts/src/types.ts#L101-L107: correct thetimeoutMsdoc comment so it matches the chosen precedence, instead of stating that the value "Applies to every call".sdk/ts/README.md#L95-L105: correct line 98, which states that both defaults are configurable. State that the execution budget is configurable per request, or update it after the precedence fix lands.
📍 Affects 3 files
sdk/ts/src/client.ts#L232-L232(this comment)sdk/ts/src/types.ts#L101-L107sdk/ts/README.md#L95-L105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/ts/src/client.ts` at line 232, Update executeService in
sdk/ts/src/client.ts:232-232 to resolve timeouts using request options first,
then the explicitly provided constructor timeout (preserving timeoutMs: 0), and
finally the default; ensure explicitTimeoutMs records whether the constructor
option was supplied and add a unit test asserting IgniteTimeoutError.timeoutMs.
Update the timeoutMs documentation in sdk/ts/src/types.ts:101-107 to describe
the actual precedence, and revise sdk/ts/README.md:95-105 to state that
execution budgets are configurable per request.
Description
Brief description of changes.
Type of Change
Checklist
Related Issues
Fixes #(issue number)
Summary by CodeRabbit
ignite servenow uses port 9847 by default; host selection uses-H.